Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3846 +/- ##
=======================================
Coverage 98.06% 98.06%
=======================================
Files 322 322
Lines 8530 8530
=======================================
Hits 8365 8365
Misses 165 165 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
sascha-karnatz
left a comment
There was a problem hiding this comment.
Looks good so far. I have only two questions:
sascha-karnatz
approved these changes
Apr 22, 2026
Drop the AlchemyHTMLElement base class in favor of a plain HTMLElement. The base class reads innerHTML in its constructor, which violates the Web Components spec and leaves cloned elements in an inconsistent state when jQuery's clone(true) walks the subtree. Spinner does not need any of the base class's abstractions: its size and color attributes are set once at element creation (via the JS Spinner wrapper or inline HTML) and never mutated afterwards, so rendering only needs to run on connection.
Drop the AlchemyHTMLElement base class in favor of a plain HTMLElement. The base class reads innerHTML in its constructor, which violates the Web Components spec and leaves cloned elements in an inconsistent state when jQuery's clone(true) walks the subtree. Overlay only needs to render once on connection. Guard the text interpolation with a nullish coalescing fallback so a missing text attribute no longer renders the literal string "null".
Drop the AlchemyHTMLElement base class in favor of a plain HTMLElement. The base class reads innerHTML in its constructor, which violates the Web Components spec and leaves cloned elements in an inconsistent state when jQuery's clone(true) walks the subtree. Replace the maxChars static property with a simple getter that falls back to 60 when the max-chars attribute is not set. The downstream comparison with charLength still works because JavaScript's relational operators coerce numeric strings to numbers.
Drop the AlchemyHTMLElement base class in favor of a plain HTMLElement. The base class reads innerHTML in its constructor, which violates the Web Components spec and leaves cloned elements in an inconsistent state when jQuery's clone(true) walks the subtree. Bail out of connectedCallback after the async locale import if the element was disconnected in the meantime. Without this guard, a fast drag-drop would leak a flatpickr calendar onto a detached input because the drop ghost gets disconnected before the import resolves. Also guard disconnectedCallback with optional chaining so a teardown before initialization does not throw.
Dropping the AlchemyHTMLElement base class avoids the constructor-time reading of innerHTML and attribute processing, which violated the Web Components spec and contributed to the SortableJS drag-clone crash. Attributes are now read through getters, Select2 is initialized in connectedCallback with an isConnected guard after the async locale import, and the instance is destroyed in disconnectedCallback.
Dropping the AlchemyHTMLElement base class avoids reading innerHTML at construction time and rewriting it during render, which violated the Web Components spec and contributed to the SortableJS drag-clone crash. The spinner is now appended via insertAdjacentHTML with an idempotent guard, so re-inserting the element does not stack multiple spinners. All setup (classname, min-height, editor hiding, observer, theme listener) is performed in connectedCallback.
Dropping the AlchemyHTMLElement base class avoids the constructor-time innerHTML capture and attribute processing. The file input and dropzone listeners, plus the self-registered Alchemy.upload.successful handler, are now wired in connectedCallback. The external dropzone element's listeners are removed in disconnectedCallback, since they would otherwise hold closures that keep the Uploader alive after removal. The dropzone attribute is exposed via a plain getter. FileUpload and Progress receive the same treatment. FileUpload now sets its initial "in-progress" status through the status setter in initialize(), so a status that is set before the element is appended (e.g. status = "failed" in tests) is no longer clobbered when the element connects. Progress pulls its template out into a module-level function and uses private methods and fields for the internals that are not part of the public contract.
No component extends it anymore after moving every custom element to plain HTMLElement and connectedCallback. The base class encouraged the very constructor-time DOM work that violated the Web Components spec and broke cloneNode (SortableJS drag and drop). Dropping it removes the tempt to repeat that pattern and the documentation now points new components at extending HTMLElement directly.
cbcbe05 to
830b02e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this pull request for?
The AlchemyHTMLElement was introduced to abstract some features for customElements into a shared base class. Mostly it was used to add reactivity to properties and attributes, but it turned out we never used is much and it just added overhead. Since it also queries the DOM in the constructor, something that's against the Custom Elements spec - we decided to remove it and inherit directly from HTMLElement instead.
Checklist